Skip to content

fix(watcher): skip context when Microcks client cannot be created - #482

Open
SAY-5 wants to merge 1 commit into
microcks:masterfrom
SAY-5:fix-watcher-nil-client
Open

fix(watcher): skip context when Microcks client cannot be created#482
SAY-5 wants to merge 1 commit into
microcks:masterfrom
SAY-5:fix-watcher-nil-client

Conversation

@SAY-5

@SAY-5 SAY-5 commented Jun 24, 2026

Copy link
Copy Markdown

Problem

In TriggerImport, when connectors.NewClient fails it logs the error but
keeps going, so the code reaches mc.UploadArtifact with a nil client and the
watcher process panics with a nil pointer dereference (#475). This happens for
example when a watch entry references a context that cannot be resolved.

Fix

continue to the next context after logging the connection error, so a failed
context is skipped instead of crashing the daemon.

Added a regression test in pkg/watcher that drives TriggerImport with an
unresolvable context; it panics without the change and passes with it.

Fixes #475

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 30 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. Microcks is a Cloud Native Computing Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@Caesarsage

Copy link
Copy Markdown
Contributor

Thank you @aniket866

This issue has been fixed on master — TriggerImport now continues to the next context when connectors.NewClient fails, so the nil client can never reach UploadArtifact.

I believe this can be closed.

@github-actions github-actions Bot removed the stale State due to inactivity label Aug 17, 2026
@Vaishnav88sk

Copy link
Copy Markdown

Same comment: #496 (comment)

@Caesarsage

Copy link
Copy Markdown
Contributor

You're absolutely right, @Vaishnav88sk, thanks for checking, and apologies for the noise. I re-read the full control flow and my comment was wrong: the continue only exists in the else branch (no-config path via NewMicrocksClient). The config-path branch logs the NewClient error and falls through to mc.UploadArtifact on a nil client — exactly the #475 scenario. So #475 is still valid and this PR still applies.

@Vaishnav88sk

Copy link
Copy Markdown

You're absolutely right, @Vaishnav88sk, thanks for checking, and apologies for the noise. I re-read the full control flow and my comment was wrong: the continue only exists in the else branch (no-config path via NewMicrocksClient). The config-path branch logs the NewClient error and falls through to mc.UploadArtifact on a nil client — exactly the #475 scenario. So #475 is still valid and this PR still applies.

Thanks for the clarification. Could you please recheck the comments yu made previously on issues/PRs about the closure. I think there are some that not yet solved or needed.

@Caesarsage

Copy link
Copy Markdown
Contributor

You're absolutely right, @Vaishnav88sk, thanks for checking, and apologies for the noise. I re-read the full control flow and my comment was wrong: the continue only exists in the else branch (no-config path via NewMicrocksClient). The config-path branch logs the NewClient error and falls through to mc.UploadArtifact on a nil client — exactly the #475 scenario. So #475 is still valid and this PR still applies.

Thanks for the clarification. Could you please recheck the comments yu made previously on issues/PRs about the closure. I think there are some that not yet solved or needed.

Done — I went back over every comment I posted, re-verifying each against master with full code reads this time.

Everything held up so far: importDirectoryPartialFailure for #473/#379, reference-counting deleteContext for #421/#427, ResolveContext in logout for #419, waitForReady in start for #303/#311, ExactArgs + --output in test for #446/#332, resp.Body + regression test for #337/#375, and zero panic( sites left in pkg/connectors for the #489 family.

One refinement rather than a correction: the "client_secret gap" I noted on #265/#333 turned out to be bigger — it's now properly documented in #503 (JSON bodies bypass redaction), with a fix in review at #504.

I am still reviewing pending ones which I will continue later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nil Pointer Dereference in Watcher Executor -> dereference panic.

3 participants